fix: record STEP_SUSPENDED for inline step_hook suspensions (stranded resume)#550
Merged
yasha-dev1 merged 3 commits intoJul 10, 2026
Merged
Conversation
When a step suspended via step_hook() on the inline path (a force_local step, or the local runtime) no STEP_SUSPENDED event was recorded — only the Celery step worker recorded one. On resume, replay saw the step's STEP_STARTED without a STEP_SUSPENDED, treated the step as still in progress, and re-suspended forever, stranding the run. - Record STEP_SUSPENDED in the engine immediately after WORKFLOW_SUSPENDED on every inline path via a single deduplicated writer (record_step_suspended_if_needed). Correct ordering by construction, no polling. Wired into the local runtime (start/resume) and the Celery orchestration handlers (start/resume/recover/child) for force_local steps. The engine's _execute_workflow_local records it too. - step_hook() now attaches the deterministic step id and step name (not the composite run_id:step_id checkpoint key) to its SuspensionSignal, so the recorded STEP_SUSPENDED matches STEP_STARTED and replay clears the in-progress state. Plumbed through set_step_execution_context. - Keep the Celery _record_step_suspended recorder for dispatched steps but short-circuit it with a has_event dedup so it never double-records. - Extend the fast-answer race guard to cover step_hook: suspensions (a hook answered from within on_created, before the suspension bookkeeping exists) on both runtimes, so the answer is never lost. - Add public WorkflowContext.get_step_hook_counter/set_step_hook_counter so apps can restore the deterministic hook-id counter when re-executing a checkpointed step across a process boundary. Folded-in fix: InMemoryStorageBackend.get_events compared the EventType enum against a list of string type values, so type-filtered queries (and thus has_event) silently matched nothing. Compare on .value. The dedup and fast-answer guards above depend on has_event working on the in-memory harness. Adds regression tests: local-runtime completion after resume_hook, crash-replay re-execution without duplicate HOOK_CREATED, fast-answer from on_created, and the counter API round-trip / hook-id reproduction.
The STEP_SUSPENDED dedup skipped recording once ANY STEP_SUSPENDED existed for a step_id. But a step can suspend multiple times on the same deterministic step_id — sequential step_hook() rounds within one step, and crash-replay re-suspension. From the second round on, a fresh STEP_STARTED was recorded with no matching STEP_SUSPENDED, so replay kept the step in _steps_in_progress, the wrapper re-took the is_step_in_progress branch, and the run re-suspended forever (stranded in SUSPENDED). The same over-dedup regressed the Celery step-worker path, which previously recorded un-deduped per suspension. Replace the "a STEP_SUSPENDED ever exists" check with step_suspended_already_recorded(): a STEP_SUSPENDED counts only if it appears after the most recent STEP_STARTED for the step_id (one type-filtered, sequence-ordered get_events scan). Both the inline writer (record_step_suspended_if_needed) and the Celery recorder (_record_step_suspended) now share this predicate, so both paths record one event per suspension round and still dedup within a round. Adds regression tests: two sequential step_hook rounds in one step complete on the local runtime (asserting one STEP_SUSPENDED per suspended STEP_STARTED cycle), and a crash-replay variant that replays with a fresh executor across the second suspension and completes after the second answer.
Cover the stranded-resume fix through a real FileStorageBackend, resuming across freshly constructed backend instances (config reset between phases) to reproduce the cross-process replay that the in-memory unit tests do not exercise: suspend->persist->resume completes, crash-replay without duplicate hook, two sequential hooks on one step_id, fast-answer race, and repeated answer-less replay staying cleanly resumable. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
A step that suspends via
step_hook()on the inline execution path (aforce_local=Truestep, or the local runtime) never gets aSTEP_SUSPENDEDevent — only the Celery step worker records one (celery/tasks.py:_record_step_suspended). On resume, replay sees the step'sSTEP_STARTEDwith noSTEP_SUSPENDED, treats it as still in progress, and re-suspends — forever. The run is stranded.Empirically confirmed downstream (FlowHunt hook-as-tool, QualityUnit/urlslab-app#6649), which currently ships an app-side workaround that pre-records the event before every
step_hookcall. This PR makes that workaround unnecessary.Repro on
main(new regression test, red before this change):Changes
1. Record
STEP_SUSPENDEDfor inlinestep_hooksuspensions. New single deduped writerengine/executor.record_step_suspended_if_needed(guards withhas_event), called from the live inline sites: bothruntime/local.pyhandlers and the 4 Celery orchestration handlers (start/resume/recover/child) forforce_localsteps. The existing Celery step-worker recorder is kept, now deduped through the same guard.SuspensionSignalcarried the compositerun_id:step_idcheckpoint key and nostep_name, while replay matches on the bare deterministic step id — an event written from the signal as-is would never clear the in-progress state. The deterministicstep_id/step_nameare now plumbed through the step execution context (step_checkpoint.pycontextvars) and attached to both signal raises, with a prefix-strip fallback.engine/executor._execute_workflow_localturned out to be dead code (zero callers); it is wired up anyway for consistency, but the fix lives at the live sites.2. Fast-answer race guard for
step_hook:. The existing guards incelery/tasks.pymatchedreason.startswith("hook:")only; extended tostep_hook:. The local runtime had no guard at all — and itsresume_hook→schedule_resume→resume_workflowchain is synchronous, so an answer delivered from withinon_createdtriggered a nested resume that couldn't progress. Added_hook_already_answered+ re-drive in bothruntime/local.pyhandlers.3. Public step-hook counter API.
ctx.get_step_hook_counter()/ctx.set_step_hook_counter(value)on the workflow context, so apps that checkpoint a step and re-execute it in a fresh process can restore the counter and reproduce the same deterministic hook id — previously only possible by poking the private_step_hook_counter.Folded-in fix:
InMemoryStorageBackend.get_eventscompared theEventTypeenum against string values (e.type in event_types) — never equal, so type-filtered queries returned nothing andhas_eventwas alwaysFalseon the in-memory backend. Now comparese.type.value, matching the file/sqlite/postgres backends. The dedup and race guards above depend on it.Tests
4 new regression tests in
tests/unit/test_step_hook.py(local-runtime suspend→resume completion, crash-replay re-execution without duplicateHOOK_CREATED, fast answer fromon_created, counter round-trip + hook-id reproduction). Test 1 verified red on unpatchedmainfirst.Full suite: 889 passed, 21 skipped (skips are pre-existing infra-gated: Redis/optional drivers).
ruff check+ruff format --checkclean; mypy clean on changed modules.Caveat: the Celery-path changes are covered by the existing Celery unit tests (green) — the end-to-end dispatched/force_local integration tests need live Redis/Celery and were skipped in this environment.
Version left at 0.3.7 per RELEASING.md (maintainer-driven bumps).